-
Notifications
You must be signed in to change notification settings - Fork 736
feat(smus): UX improvements for connecting running spaces from toolkit #8261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(smus): UX improvements for connecting running spaces from toolkit #8261
Conversation
|
1a5a7cf to
3cfbdfe
Compare
laileni-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its better to add change log for this !
| } | ||
|
|
||
| // Get current instance type | ||
| const requestedResourceSpec = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit : "requestedResourceSpec" - Is this called in the update flow? If not, the "requested" part is misleading.
| ) | ||
| } else { | ||
| // Only remote access needs to be enabled | ||
| prompt = `This space requires remote access to be enabled.\nWould you like to restart the space and connect?\nAny unsaved work will be lost.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to a constant where existing error messages are managed please?
| }) | ||
|
|
||
| // Update progress message | ||
| progress.report({ message: 'Starting the space' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit : Can maybe include the new instanceType in the case we are also upgrading instance type.
| let baseTooltip = '' | ||
| if (this.isSMUSSpace) { | ||
| return `**Space:** ${spaceName} \n\n**Application:** ${appType} \n\n**Instance Type:** ${instanceType}` | ||
| baseTooltip = `**Space:** ${spaceName} \n\n**Application:** ${appType} \n\n**Instance Type:** ${instanceType}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this!
7a4f4cd to
03358a7
Compare
vpbhargav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test thoroughly for both SMUS and SMAI, thanks for making this improvement!
packages/toolkit/.changes/next-release/feature-smus-ux-improvements.json
Show resolved
Hide resolved
Head branch was pushed to by a user without write access
c0b5314 to
e0278f5
Compare
|
|
||
| // Setup test window to handle confirmation dialog | ||
| getTestWindow().onDidShowMessage((message) => { | ||
| if (message.message.includes('remote access')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would find this easier to understand what we are testing if I could see a little bit more of the message we are matching. If we have a constant for the message, we can simply reference it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e0278f5 to
0e8e53a
Compare
Problem
-- Users expected to be able to connect to any running space
-- No visual indication that connection was possible
-- Users didn't understand why some spaces were "connectable" and others weren't
Solution
Current User Exp
Screen.Recording.2025-11-05.at.3.23.39.PM.mov
feature/xbranches will not be squash-merged at release time.